-
Notifications
You must be signed in to change notification settings - Fork 2.3k
Add openapi v3 #5627
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Add openapi v3 #5627
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi, thanks for taking the time to look into this. It's going to be hard for me to review this the way it's currently written. It seems to be based a lot on the existing generator. Could you talk to me about the process you used to build this? I explicitly said that I want us to start from scratch with this generator and not rely on existing code, if we can avoid it. I would remove basically all the flags too and add them piece by piece. Did you use AI to build this?
Hi, |
I'm also a little bit unfamiliar with bazel. I will fix the bazel config to not generate the swagger files |
f912d21
to
c827f3a
Compare
@johanbrandhorst could you please take another look at it? I deleted the borrowed parts and rewrote them. |
I only implemented the flags that we use in our company for V2. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I haven't looked at the code, but I ran a simple test:
go install ./protoc-gen-openapiv3
- Add a new stanza to
buf.gen.yaml
for theprotoc-gen-openapiv3
plugin - Run
buf generate
It crashed with a stack overflow, I think you have a broken recursion somewhere. The stack trace is:
runtime: goroutine stack exceeds 1000000000-byte limit
runtime: sp=0x4020c00380 stack=[0x4020c00000, 0x4040c00000]
fatal error: stack overflow
runtime stack:
runtime.throw({0x55bc01?, 0x3b1b8?})
/usr/local/go/src/runtime/panic.go:1096 +0x38 fp=0x40000ade70 sp=0x40000ade40 pc=0x7ecc8
runtime.newstack()
/usr/local/go/src/runtime/stack.go:1107 +0x45c fp=0x40000adfb0 sp=0x40000ade70 pc=0x65cbc
runtime.morestack()
/usr/local/go/src/runtime/asm_arm64.s:342 +0x70 fp=0x40000adfb0 sp=0x40000adfb0 pc=0x83a00
goroutine 1 gp=0x40000021c0 m=4 mp=0x4000098008 [running]:
runtime.mallocgcSmallScanNoHeader(0x20?, 0x474f40?, 0x1?)
/usr/local/go/src/runtime/malloc.go:1340 +0x344 fp=0x4020c00380 sp=0x4020c00380 pc=0x23a64
runtime.mallocgc(0x20, 0x474f40, 0x1)
/usr/local/go/src/runtime/malloc.go:1058 +0x88 fp=0x4020c003b0 sp=0x4020c00380 pc=0x7c8b8
runtime.growslice(0x4020c004a8, 0x0?, 0x0?, 0x0?, 0x474f40)
/usr/local/go/src/runtime/slice.go:272 +0x464 fp=0x4020c00410 sp=0x4020c003b0 pc=0x81314
github.com/grpc-ecosystem/grpc-gateway/v2/internal/descriptor.(*Message).FQMN(0x4000423ef0)
/home/johan/src/grpc-ecosystem/grpc-gateway/internal/descriptor/types.go:101 +0x70 fp=0x4020c004c0 sp=0x4020c00410 pc=0x2ca150
github.com/grpc-ecosystem/grpc-gateway/v2/protoc-gen-openapiv3/internal/genopenapiv3.(*fileGenerator).generateMessageSchema(0x4040bffd40, 0x4000423ef0, {0x0, 0x0, 0x1?})
/home/johan/src/grpc-ecosystem/grpc-gateway/protoc-gen-openapiv3/internal/genopenapiv3/file_generator.go:72 +0x38 fp=0x4020c00750 sp=0x4020c004c0 pc=0x3e7928
github.com/grpc-ecosystem/grpc-gateway/v2/protoc-gen-openapiv3/internal/genopenapiv3.(*fileGenerator).generateMessageSchemaRef(0x4040bffd40, 0x4000423ef0)
/home/johan/src/grpc-ecosystem/grpc-gateway/protoc-gen-openapiv3/internal/genopenapiv3/file_generator.go:66 +0xf4 fp=0x4020c007d0 sp=0x4020c00750 pc=0x3e7824
github.com/grpc-ecosystem/grpc-gateway/v2/protoc-gen-openapiv3/internal/genopenapiv3.(*fileGenerator).generateFieldTypeSchema(0x4040bffd40, 0x400042f200, {0x4010912140, 0x28})
/home/johan/src/grpc-ecosystem/grpc-gateway/protoc-gen-openapiv3/internal/genopenapiv3/file_generator.go:229 +0x1b4 fp=0x4020c00840 sp=0x4020c007d0 pc=0x3e8944
github.com/grpc-ecosystem/grpc-gateway/v2/protoc-gen-openapiv3/internal/genopenapiv3.(*fileGenerator).generateFieldDoc(0x4040bffd40, 0x40004f0d60)
/home/johan/src/grpc-ecosystem/grpc-gateway/protoc-gen-openapiv3/internal/genopenapiv3/file_generator.go:203 +0x304 fp=0x4020c008b0 sp=0x4020c00840 pc=0x3e8754
github.com/grpc-ecosystem/grpc-gateway/v2/protoc-gen-openapiv3/internal/genopenapiv3.(*fileGenerator).generateMessageSchema(0x4040bffd40, 0x4000423ef0, {0x0, 0x0, 0x1?})
/home/johan/src/grpc-ecosystem/grpc-gateway/protoc-gen-openapiv3/internal/genopenapiv3/file_generator.go:88 +0x1d0 fp=0x4020c00b40 sp=0x4020c008b0 pc=0x3e7ac0
github.com/grpc-ecosystem/grpc-gateway/v2/protoc-gen-openapiv3/internal/genopenapiv3.(*fileGenerator).generateMessageSchemaRef(0x4040bffd40, 0x4000423ef0)
/home/johan/src/grpc-ecosystem/grpc-gateway/protoc-gen-openapiv3/internal/genopenapiv3/file_generator.go:66 +0xf4 fp=0x4020c00bc0 sp=0x4020c00b40 pc=0x3e7824
github.com/grpc-ecosystem/grpc-gateway/v2/protoc-gen-openapiv3/internal/genopenapiv3.(*fileGenerator).generateFieldTypeSchema(0x4040bffd40, 0x400042f200, {0x4010907fc0, 0x28})
/home/johan/src/grpc-ecosystem/grpc-gateway/protoc-gen-openapiv3/internal/genopenapiv3/file_generator.go:229 +0x1b4 fp=0x4020c00c30 sp=0x4020c00bc0 pc=0x3e8944
github.com/grpc-ecosystem/grpc-gateway/v2/protoc-gen-openapiv3/internal/genopenapiv3.(*fileGenerator).generateFieldDoc(0x4040bffd40, 0x40004f0d60)
/home/johan/src/grpc-ecosystem/grpc-gateway/protoc-gen-openapiv3/internal/genopenapiv3/file_generator.go:203 +0x304 fp=0x4020c00ca0 sp=0x4020c00c30 pc=0x3e8754
github.com/grpc-ecosystem/grpc-gateway/v2/protoc-gen-openapiv3/internal/genopenapiv3.(*fileGenerator).generateMessageSchema(0x4040bffd40, 0x4000423ef0, {0x0, 0x0, 0x1?})
/home/johan/src/grpc-ecosystem/grpc-gateway/protoc-gen-openapiv3/internal/genopenapiv3/file_generator.go:88 +0x1d0 fp=0x4020c00f30 sp=0x4020c00ca0 pc=0x3e7ac0
github.com/grpc-ecosystem/grpc-gateway/v2/protoc-gen-openapiv3/internal/genopenapiv3.(*fileGenerator).generateMessageSchemaRef(0x4040bffd40, 0x4000423ef0)
/home/johan/src/grpc-ecosystem/grpc-gateway/protoc-gen-openapiv3/internal/genopenapiv3/file_generator.go:66 +0xf4 fp=0x4020c00fb0 sp=0x4020c00f30 pc=0x3e7824
github.com/grpc-ecosystem/grpc-gateway/v2/protoc-gen-openapiv3/internal/genopenapiv3.(*fileGenerator).generateFieldTypeSchema(0x4040bffd40, 0x400042f200, {0x4010907e00, 0x28})
...
/home/johan/src/grpc-ecosystem/grpc-gateway/protoc-gen-openapiv3/internal/genopenapiv3/file_generator.go:66 +0xf4 fp=0x4040bffbe0 sp=0x4040bffb60 pc=0x3e7824
github.com/grpc-ecosystem/grpc-gateway/v2/protoc-gen-openapiv3/internal/genopenapiv3.(*fileGenerator).generateFileSpec(0x4040bffd40, 0x40004ee240)
/home/johan/src/grpc-ecosystem/grpc-gateway/protoc-gen-openapiv3/internal/genopenapiv3/file_generator.go:44 +0x288 fp=0x4040bffc70 sp=0x4040bffbe0 pc=0x3e76a8
github.com/grpc-ecosystem/grpc-gateway/v2/protoc-gen-openapiv3/internal/genopenapiv3.(*generator).Generate(0x400000c2b8, {0x40004f1e00, 0x4, 0x400024d800?})
/home/johan/src/grpc-ecosystem/grpc-gateway/protoc-gen-openapiv3/internal/genopenapiv3/generator.go:44 +0x150 fp=0x4040bffdb0 sp=0x4040bffc70 pc=0x3ea8e0
main.main()
/home/johan/src/grpc-ecosystem/grpc-gateway/protoc-gen-openapiv3/main.go:89 +0x72c fp=0x4040bfff40 sp=0x4040bffdb0 pc=0x3ee05c
runtime.main()
/usr/local/go/src/runtime/proc.go:283 +0x284 fp=0x4040bfffd0 sp=0x4040bfff40 pc=0x4c064
runtime.goexit({})
/usr/local/go/src/runtime/asm_arm64.s:1223 +0x4 fp=0x4040bfffd0 sp=0x4040bfffd0 pc=0x85c44
hank you for testing and reviewing this. To help me investigate, could you please provide the .proto file you used? My own testing was done with the standard protoc command, which I expected to behave similarly to buf.gen. It seems I've missed an edge case. My leading theory is that the issue is related to a "resurrection check" from v2, which my current implementation doesn't include. |
I'm not sure exactly which proto file it is, as I said, I just ran it on the proto files in this repo (using |
"recursion check" sorry. :) |
@johanbrandhorst Hi again, |
Could you rebase this branch on |
c96b3bb
to
93d2dcb
Compare
@johanbrandhorst I rebased and changed the status to waiting for review. I believe it requires an approval to run: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm confused, shouldn't there be some generated openapiv3 files?
package genopenapiv3 | ||
|
||
import ( | ||
"github.com/getkin/kin-openapi/openapi3" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd prefer not to add a third party dependency like this, could we define these types ourselves?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we can. I talk about using kin-openapi before starting the implementation. The library is well maintained an other repositories like https://github.com/oapi-codegen/oapi-codegen use it as well. It think the main benefit would be in the tests where there are some cool functions like validate and JSONLookup. But if you think the cons outweigh the pros I will remove it and use our own types. The bazel was giving me hell because of it as well...
"google.golang.org/protobuf/proto" | ||
|
||
"github.com/grpc-ecosystem/grpc-gateway/v2/internal/descriptor" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Merge these import groups
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sure.
// ID assigned by [email protected] for | ||
// gRPC-Gateway project. | ||
// | ||
// All IDs are the same, as assigned. It is okay that they are the same, as | ||
// they extend different descriptor messages. | ||
OpenAPI openapiv3_document = 1043; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If this number is the same as the protoc-gen-openapiv2, it means users can't use this together with openapiv2. We will need a new number. For now, can we use 99999, but if/when we merge this, I will request a new number from the global extension registry. See https://protobuf.dev/programming-guides/proto2/#customoptions for more information.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If this number is the same as the protoc-gen-openapiv2, it means users can't use this together with openapiv2. We will need a new number. For now, can we use 99999, but if/when we merge this, I will request a new number from the global extension registry. See https://protobuf.dev/programming-guides/proto2/#customoptions for more information.
yes sorry. I forgot to change it.
Congratulations on your graduation by the way :) |
93d2dcb
to
0b40f62
Compare
Thanks:) The post graduation paper work was holding me up sorry for the late reply |
The bazel image doesn't run on my ARM laptop, I have to run bazel on a x86_64 VM, I forgot to run the bazel commands I will run and push. |
@johanbrandhorst I believe I have resolved the comments. |
References to other Issues or PRs
This feature was request in #441.
Have you read the Contributing Guidelines?
Brief description of what is fixed or changed
This is a minimal implementation (as I'm writing this description a draft) for openapiv3 protoc plugin.
Other comments
I plan to add tests and complete other features. I just want to get this look at before I go all in.
Right now it generates OpenAPI spec version 3, using
kin-openapi
library.